Fixes #37995 - High memory rendering Arf Report index page #584
Conversation
Avoid using includes() with nested associations and "order by" together. Otherwise, includes() will use join tables instead and Rails somehow create many objects and high memory consumption.
hao-yu
force-pushed
the
fixes_37995_arfreport_index_page
branch
from
eab73eb
to
5112cfc
Compare
hao-yu
changed the title
|
Before patching: VS After patching |
There was a problem hiding this comment.
Thanks, @hao-yu !
Sorry for not checking this out earlier. It indeed reduces the count of allocations and drops rendering time. Also, I didn't find any drawbacks of this optimization: everything seems to be working still.
@adamruzicka, any objections?
Some observations unrelated to this PR: I wanted to test this approach in other places, but it seems we already follow the same rule: search and paginate at first, then use .inlcudes/.preload/etc. At least most of the time. There are some exceptions, such as policies in this plugin and the host page in the Foreman itself.
I've tried to make some presumably unnecessary small changes to hosts_controller.rb and users_controller.rb in Foreman and even with small set of entities I've noticed:
- Hosts page without patch:
- Completed 200 OK in 323ms (Views: 241.1ms | ActiveRecord: 40.2ms | Allocations: 210751)
- Hosts page with patch:
- Completed 200 OK in 305ms (Views: 195.0ms | ActiveRecord: 40.8ms | Allocations: 233857)
- Users page with patch:
- Completed 200 OK in 55ms (Views: 34.1ms | ActiveRecord: 3.8ms | Allocations: 57181)
- Users page without patch:
- Completed 200 OK in 74ms (Views: 47.7ms | ActiveRecord: 11.1ms | Allocations: 74892)
Even though it somehow increased the number of allocations for hosts page, it still reduced the time of the response... Doesn't seems right, but that was just poking around, not a real patch.
For users page though, it improved both numbers.
There was a problem hiding this comment.
To be honest I'm not exactly fond of the final sort being done in ruby, but we can live with that, considering it is just a page's worth of results. In the end, 5x improvement is rather nice.
There's on styling change that would be nice to have resolved before merging, but we can do that ourselves.
|
Thanks, @hao-yu and @adamruzicka ! |
Avoid using includes() with nested associations and "order by" together. Otherwise, includes() will use join tables instead and Rails somehow create many objects and high memory consumption.
For more information, please refer to https://projects.theforeman.org/issues/37995